Fix NFA compilation failure for grammars with shared optional prefixes#2629
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an @typeagent/action-grammar NFA compilation failure triggered by optimizer output for grammars whose top-level alternatives share a fully-optional common prefix (e.g. optional “please”), and aligns the agc default optimization preset with NFA compatibility (agent-server’s default grammar system).
Changes:
- Teach the NFA compiler to derive an implicit forwarding value for value-less multi-part rules when exactly one part is variable-bearing (and error clearly when ambiguous).
- Add an
nfaSafeOptimizationspreset (recommended minustailFactoring/promoteTailRulesParts) and switchagc’s default optimizations to that preset. - Add regression tests covering the repro, the still-unsupported
tailCallpath, and the ambiguous implicit-value guard.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ts/packages/actionGrammarCompiler/src/commands/compile.ts | Switch agc default optimizations to nfaSafeOptimizations for NFA compatibility. |
| ts/packages/actionGrammar/test/nfaFactoredPrefixValues.spec.ts | New regression tests for shared optional-prefix factoring, tailCall refusal, and ambiguous implicit value. |
| ts/packages/actionGrammar/src/nfaCompiler.ts | Derive implicit values for value-less multi-part rules with a single variable-bearing part; improve diagnostics. |
| ts/packages/actionGrammar/src/index.ts | Export nfaSafeOptimizations from the package entrypoint. |
| ts/packages/actionGrammar/src/grammarOptimizer.ts | Introduce the nfaSafeOptimizations preset and document why it exists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
curtisman
reviewed
Jul 9, 2026
…es' into dev/georgeng/fix_nfa_with_subrules
curtisman
reviewed
Jul 9, 2026
Line-number shifts from the NFA fix broke file:line matches for pre-existing baselined offenders (nfaCompiler.ts, grammarOptimizer.ts). Regenerated via 'npm run code-complexity:update-exceptions'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
curtisman
reviewed
Jul 9, 2026
curtisman
reviewed
Jul 9, 2026
curtisman
reviewed
Jul 9, 2026
curtisman
reviewed
Jul 9, 2026
…om/microsoft/TypeAgent into dev/georgeng/fix_nfa_with_subrules
curtisman
reviewed
Jul 13, 2026
curtisman
reviewed
Jul 13, 2026
curtisman
reviewed
Jul 14, 2026
curtisman
reviewed
Jul 14, 2026
curtisman
reviewed
Jul 14, 2026
…om/microsoft/TypeAgent into dev/georgeng/fix_nfa_with_subrules
curtisman
reviewed
Jul 14, 2026
curtisman
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See issue #2499
Problem
The NFA compiler rejected grammars where multiple top-level alternatives share
a common optional prefix (e.g. = | ; =
foo -> ...;
= (please)?; ). The optimizer's factorCommonPrefixes pass can produce a
value-less, multi-term rule in this shape, which the NFA compiler had no
general way to derive a value for — it only special-cased a narrower "single
wildcard/number part" shape.
Root-cause fix
• nfaCompiler.ts now derives a rule's effective value (explicit ->
expression, or implicit single-variable-part forwarding) using the same
general logic the AST-based optimizer already used, instead of its old,
narrower special case.
• Extracted this rule-value-derivation logic into a new shared module,
grammarValueDeriver.ts — a single source of truth now used by both the
grammar optimizer and the NFA compiler (previously each had its own
slightly different implementation).
• Side effect of consolidation: nested rules with a single bound variable and
no explicit value now correctly forward that value in the NFA path instead
of silently discarding it.
• Harmonized error messages for rules with no value and no single
variable-bearing part to forward, across both the "ambiguous" and
"missing" throw paths.
Follow-up hardening
• Hoisted the separate tailCall (NYI tail-RulesPart) rejection check to a
single, earliest, unconditional location ( normalizeRule ), run before any
rule rewriting — closing a gap where one rule shape (a single-part
tailCall rule) could slip past the check and surface a confusing,
differently-worded error later in compilation.
• Unified the optimizer's getImplicitDefaultValue onto the same shared
deriveEffectiveValue , removing a second, redundant implementation.
Testing
• Added targeted unit tests for the shared derivation helper and regression
tests for previously-unreachable error paths (0-part / 1-part rules with
no value, and the single-part tailCall shape).
• Cleaned up stale comments/tests referencing removed or relocated
functions; verified no redundant test coverage was introduced.
• Full suite: 75/75 suites, 16153/16155 tests passing (2 skipped), clean
build, prettier clean.